Skip to content

fix: merge transaction context into hook context evaluation context (#521) #523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 23, 2025
Merged

fix: merge transaction context into hook context evaluation context (#521) #523

merged 1 commit into from
Jul 23, 2025

Conversation

@kouk
Copy link
Contributor

@kouk kouk commented Jul 18, 2025

merge transaction context into hook context evaluation context

Currently during flag evaluation the transaction context is merged into the evaluation context here:

python-sdk/openfeature/client.py

Line 778 in 288bd6b

merged_context = self._before_hooks_and_merge_context(

However the hook_context.evaluation_context is created before this, so it doesn't contain the attributes or targeting key that exists in the propagated transaction context. This is unexpected. It is also different than what the JS implementation does.

In this PR:

  1. The transaction context is merged with the evaluation context in _establish_hooks_and_provider before creating the hook context.
  2. The _before_hooks_and_merge_context method is renamed as _run_before_hooks_and_update_context to better reflect its updated functionality which is now restricted to just adding the before hooks results to the context.
  3. Renamed a variable in _run_before_hooks_and_update_context which was inaptly named IMHO. The result of the before hooks is not the "invocation context".
  4. Adds a test to verify the transaction context is properly merged into the hook context's evaluation context

This ensures that hooks have access to the complete evaluation context, including any attributes or targeting key from the transaction context.

Related Issues

Fixes #521

Notes

Currently, both the Python sdk and the JS sdk do not update the hook context's evaluation_context field after running the before hooks. This is not explicitly required by the spec here: https://openfeature.dev/specification/sections/evaluation-context/#requirement-323
or here:
https://openfeature.dev/specification/sections/hooks

However it seems to me to be a safe assumption that if the before hooks update the evaluation context then the hook context should reference this updated evaluation context instead of referencing the evaluation context as it was before the before hooks ran. I can open a different issue about this

@kouk kouk requested review from a team as code owners July 18, 2025 16:10
@kouk
Copy link
Contributor Author

kouk commented Jul 18, 2025

@thomaspoignant would be interested in your opinion on the "Notes" section above.

@kouk kouk force-pushed the fix/transaction-context-hooks branch from 6f9176c to 13d0f92 Compare July 18, 2025 16:17
@codecov
Copy link

codecov bot commented Jul 18, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (5652c0c) to head (5ce432f).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
+ Coverage   97.65%   97.85%   +0.19%     
==========================================
  Files          37       38       +1     
  Lines        1788     1814      +26     
==========================================
+ Hits         1746     1775      +29     
+ Misses         42       39       -3
Flag Coverage Δ
unittests 97.85% <100.00%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@beeme1mr
Copy link
Member

beeme1mr commented Jul 18, 2025

Hey @kouk, are you updating the transaction context in a before hook? You should be able to return additional context from the before hook. You can see how that work in JS. You're correct that we don't re-evaluate the transaction context after the before hook but it shouldn't be necessary because of the feature I mentioned above.

@kouk
Copy link
Contributor Author

kouk commented Jul 19, 2025

@beeme1mr AFAICT the python sdk does not do what the JS sdk does. Here's the relevant part of the JS sdk: https://github.com/open-feature/js-sdk/blob/d8bd93b6d5256445d12185d639e1cd91800a8a16/packages/server/src/client/internal/open-feature-client.ts#L329
It apparently updates the evaluation context that is referenced by the hook context (might be misunderstanding the code here so check my assumption).

OTOH the Python SDK does not do this:

python-sdk/openfeature/hook/_hook_support.py

Line 51 in 288bd6b

def before_hooks(

This PR does not currently change this, so there is a discrepancy IMHO between the JS and Python SDKs in this regard. I could update this PR or create a new one to address this discrepancy.

@leohoare
Copy link
Contributor

leohoare commented Jul 21, 2025

Changes look okay from my end, please address the linting.

However it seems to me to be a safe assumption that if the before hooks update the evaluation context then the hook context should reference this updated evaluation context instead of referencing the evaluation context as it was before the before hooks ran. I can open a different issue about this

From a consumer, that behaviour is expected but it should be decided by the spec.
We could always raise an issue and take it to openfeature?

@kouk kouk force-pushed the fix/transaction-context-hooks branch from 4378d1d to f08fa5f Compare July 22, 2025 05:45
gruebel
gruebel reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, nice work 🍻 left a small comment

This was referenced Jul 22, 2025
Closed
Open
@kouk
Copy link
Contributor Author

kouk commented Jul 22, 2025

Opened two issues in the spec repo:
open-feature/spec#328
open-feature/spec#329

@kouk kouk force-pushed the fix/transaction-context-hooks branch from 56e968e to 5ce432f Compare July 22, 2025 06:54
@kouk
Copy link
Contributor Author

kouk commented Jul 22, 2025

@leohoare it occurred to me after reading your comment that the current fix is also not something that is explicitly specified by the spec. Although it seems pretty obvious to me perhaps the spec could be updated to say this explicitly: open-feature/spec#328

@gruebel gruebel merged commit a5b3aa9 into open-feature:main Jul 23, 2025
15 checks passed
@openfeaturebot openfeaturebot mentioned this pull request Jul 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@beeme1mr beeme1mr beeme1mr approved these changes

@aepfli aepfli aepfli approved these changes

@thomaspoignant thomaspoignant thomaspoignant approved these changes

@gruebel gruebel gruebel approved these changes

+1 more reviewer

@leohoare leohoare leohoare approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Transaction context should be merged into the evaluation context of the hooks

6 participants

@kouk @beeme1mr @leohoare @aepfli @thomaspoignant @gruebel